refactor(runner): route flashduty auth through bash extraEnv (drops flashduty_exec op)#50
Merged
Conversation
…ction Adds executeFlashdutyExec that fork-execs the flashduty CLI with per-call auth env, isolated from the generic bash code path. Routes TaskOpFlashdutyExec through the WebSocket handler. Phase 2 of fc-safari CLI adoption.
Phase 2 contract is that only the `flashduty_exec` path carries per-user Flashduty credentials (FLASHDUTY_APP_KEY, FLASHDUTY_API_BASE, ...). The generic `bash` tool must not see them. The previous executeBashCommand inherited the runner's full os.Environ(), which leaked any FLASHDUTY_* keys present in the runner process — easy to hit on a dev workstation that exports them for safari, and impossible to audit in cloud sandboxes when sandbox-manager forwards arbitrary entries. Fix: scrubFlashdutySecrets() drops every FLASHDUTY_* entry from the inherited env before bash sees it. Caller-supplied extraEnv layers on top, so explicit hand-offs still work (test fixtures + non-secret overrides unchanged). The flashduty_exec path is untouched — it relies on safari's per-call extraEnv to re-add FLASHDUTY_APP_KEY in-place. Caught in Phase 2 E2E: bash subprocess reported FLASHDUTY_APP_KEY=<key> even though Phase 2 was supposed to remove it. Re-verified with the scrub: bash sees zero FLASHDUTY_* entries; flashduty tool still returns real incident data via its own auth pathway.
Two CI issues caught after the flashduty_exec PR push: 1. gofmt complained about the FlashdutyExecArgs struct alignment in protocol/messages.go — comment-column re-flow after the new fields. 2. windows-latest unit-test job failed because TestExecuteFlashdutyExec_ VerbSplitsOnWhitespace shells out to /bin/echo to inspect argv. There is no PowerShell equivalent that preserves the same stdout shape, so the test is now skipped on GOOS=windows (mirrors the envd POSIX-only skips landed earlier).
…h extraEnv Replaces the dedicated flashduty_exec operation with the simpler model Safari's bash guard now drives: when Safari recognizes a `flashduty` CLI invocation, it injects FLASHDUTY_APP_KEY into the bash payload's `env` map for that one subprocess. The runner just honors extraEnv on bash like it already does. Removed surface: - `TaskOpFlashdutyExec` const + `FlashdutyExecArgs` struct (protocol) - `case TaskOpFlashdutyExec` dispatch (ws/handler.go) - `Environment.FlashdutyExec()` public + `executeFlashdutyExec()` impl - Two unit tests covering the deleted path Kept (and tightened comments): - `scrubFlashdutySecrets` on the bash path — defense-in-depth so the runner's own ambient FLASHDUTY_* never leaks into a bash subprocess even on BYOC dev workstations where the operator's shell exports them. Safari's per-call extraEnv overlay still wins the merge for genuine CLI invocations because extraEnv is layered last. - All bash output capture / 10MB cap / .outputs spill semantics unchanged. New test `TestEnvironment_Bash_ExtraEnvFlashdutyInjection` pins the overlay-wins-scrub contract end-to-end on the bash path. Trade-off vs the dedicated tool: argv-isolated `flashduty` invocations no longer exist — the CLI runs inside a real `bash -c "..."` shell, so a malicious model could in principle pipe through `xxd` or similar. Safari's bash guard (separate PR) handles that by rejecting any command that references `FLASHDUTY_APP_KEY` by name or bulk-dumps the env. Residual risk is accepted for BYOC/sandbox isolation where the runner already runs under customer-owned credentials in a customer-controlled environment.
…uty CLI) A BYOC runner executes the agent's bash commands in the customer's own environment, where `flashduty` may already be on PATH at a different version — or be an unrelated binary of the same name. Resolving the CLI via the ambient PATH is therefore not hermetic. The runner now prepends its bundled-tools directory to every bash subprocess's PATH so OUR flashduty shadows any same-named host binary. The dir is FLASHDUTY_RUNNER_BIN_DIR when set, else the directory the runner executable lives in (the cloud sandbox image and the host installer both place the CLI next to the runner binary). No-op when the dir can't be determined, so existing deployments without a bundled CLI are unaffected. This makes renaming the CLI to a private name (e.g. `fduty`) unnecessary: PATH precedence guarantees our version wins even on a bare `flashduty` invocation, so the public brand name is preserved. withBundledToolsPath is unit-tested across prepend / already-first / absent-PATH / empty-value / empty-dir cases. Note: shipping the CLI into the BYOC host install (install.sh) is tracked separately — it needs a CLI-version-pinning policy decision. The cloud sandbox already bakes the CLI next to the runner, so that path is complete.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two changes that make the Flashduty CLI work cleanly under the runner's
bashtool:flashduty_execwire op).flashdutyalways wins over any copy the BYOC host happens to have onPATH.Paired with fc-safari PR #74 (the bash guard that injects auth and rejects credential read-back).
1. Auth via bash extraEnv (supersedes
flashduty_exec)Safari recognizes
flashdutyCLI invocations in bash commands, injectsFLASHDUTY_APP_KEYinto the bash payload'senvmap for that one subprocess, and the runner just honorsenvlike it already does for bash. This supersedes the earlier Phase 2 design (dedicated runner op + dedicated safari tool) — see fc-safari PR #74 for the rationale.Removed:
protocol.TaskOpFlashdutyExec+FlashdutyExecArgs(wire op no longer needed)case TaskOpFlashdutyExec:dispatch inws/handler.goEnvironment.FlashdutyExec()public +executeFlashdutyExec()implKept (with tightened comments):
scrubFlashdutySecretsonexecuteBashCommand— defense-in-depth so the runner's own ambientFLASHDUTY_*never leaks into a bash subprocess on BYOC dev workstations where the operator's shell exports them. Safari's per-call extraEnv overlay still wins the merge for genuine CLI invocations because extraEnv is layered last..outputs/spill semantics unchanged.Added:
TestEnvironment_Bash_ExtraEnvFlashdutyInjectionpins the overlay-wins-scrub contract end-to-end on the bash path.Wire shape
Safari sends a normal bash payload; the new bit is the optional
env:{ "command": "flashduty incident list --severity Critical --progress Triggered,Processing --output-format toon", "workdir": "...", "timeout": 120, "env": { "FLASHDUTY_APP_KEY": "..." } }The runner merges
envon top ofscrubFlashdutySecrets(os.Environ())and hands the result toexec.CommandContext. The credential lives in exactly one process's envp and dies with it.2. Hermetic CLI: bundled-tools PATH precedence (commit
6c898876)Problem (Q2 from the design discussion): on a BYOC host the customer may already have their own
flashdutyonPATH(different version, different auth). If the bash subprocess resolvesflashdutyto the host copy, Safari's injectedFLASHDUTY_APP_KEYwould be handed to a binary we don't control, and version drift could break flag parsing.Fix:
executeBashCommandnow prepends the runner's bundled-tools directory to the subprocessPATH, so ourflashdutyshadows any host copy:bundledToolsDir()—FLASHDUTY_RUNNER_BIN_DIRenv override if set, elsefilepath.Dir(os.Executable())(the directory the runner binary itself lives in, where the CLI is bundled alongside it).withBundledToolsPath(env, dir)— folds the dir onto the front of the existingPATH=entry. Idempotent: no-op when the dir is already first; handles emptyPATH, empty dir, and a missingPATHentry.Added:
environment/bundled_tools_path_test.go— table test covering prepend / already-first / absent / empty-value / empty-dir / not-first.This is independent of the auth overlay: PATH resolution picks which binary runs; extraEnv supplies its credential.
Trade-off vs the dedicated tool
Argv isolation is gone —
flashduty <verb>now runs insidebash -c "..."so a malicious model could theoretically pipe stdout throughxxdor similar. Safari's bash guard (PR #74) rejects the obvious leak paths:FLASHDUTY_APP_KEY/FLASHDUTY_API_KEYby nameenv,printenv,compgen -e,/proc/*/environ)Residual risk is accepted for BYOC/sandbox where the runner already runs under customer-owned credentials in a customer-controlled environment.
Test plan
go test ./... -count=1— all green (envd / environment / permission, incl. the new PATH-precedence table)go build ./... && go vet ./...clean